finishing the chalenge #1517
finishing the chalenge #1517
Conversation
src/components/Header.tsx
Outdated
| editingTodosId: number[]; | ||
| setTodos: (todos: Todo[]) => void; | ||
| setFilteredTodos: (todos: Todo[]) => void; | ||
| setIsActiveFilter: (isActive: boolean) => void; |
There was a problem hiding this comment.
You're not using the setIsActiveFilter in the header, so it shouldn't be in the declaration of the props type
src/App.tsx
Outdated
| editingTodosId={editingTodosId} | ||
| setTodos={setTodos} | ||
| setFilteredTodos={setFilteredTodos} | ||
| setIsActiveFilter={setIsActiveFilter} |
There was a problem hiding this comment.
It has no effect to sen setIsActiveFilter if you are not using it in the header
src/App.tsx
Outdated
| const [filteredTodos, setFilteredTodos] = useState<Todo[]>([]); | ||
| const [error, setError] = useState<string | null>(null); | ||
| const [editingTodosId, setEditingTodosId] = useState<number[]>([]); | ||
| const [isActiveFilter, setIsActiveFilter] = useState(false); |
There was a problem hiding this comment.
correct me if I'm wrong. This variable refers to the state of the footer, right? Whether if it should be rendered or not. If that's so, you could name it better, to something like isFooterActive and setIsFooterActive, you don't need the Filter because it is a boolean state, so it has only two possible states, and it's better to use the Footer in the name to make ir more specific
There was a problem hiding this comment.
The way it is, it can be confusing because you actually have a Active filter on the application
src/App.tsx
Outdated
| if (filter === 'active') { | ||
| setFilteredTodos(prev => prev.filter(t => !t.completed)); | ||
| } | ||
| }, [filter, filteredTodos, todos]); |
There was a problem hiding this comment.
It's not a good idea to have the filteredTodos as a dependency here. It causes your list to not render all the todos when you change the filter from Active/Completed to All.
src/components/Header.tsx
Outdated
|
|
||
| setFilteredTodos([...todos].filter(t => t.id !== newTodoIdAvailable)); | ||
| setTodos([...todos, responseFromAddedTodo]); | ||
| setFilteredTodos([...todos, responseFromAddedTodo]); |
There was a problem hiding this comment.
This part of the code is a little confusing, you're manipolating the filteredTodos too much, making the rendered todos messy. Try imagining what you want to be rendered when you add a new todo, think of all scenarios, try adding a new todo when you have each filter on, and what you want to be rendered on the screen while adding the new todo in each case.
src/components/Header.tsx
Outdated
|
|
||
| function handleToggleAllTodos() { | ||
| const successfulUpdatedTodo: number[] = []; | ||
| const hasAllSameStatus = todos.every(t => t.completed); |
There was a problem hiding this comment.
This is not actually a mistake, just an possible improvement.
add a Complete to the name of the variable. You want it to have the value true if all the todos have status completed. When you name the varibale hasAllSameStatus it gives the impression that if the status of all the todos is not completed, it would be true as well.
| const [filteredTodos, setFilteredTodos] = useState<Todo[]>([]); | ||
| const [error, setError] = useState<string | null>(null); | ||
| const [editingTodosId, setEditingTodosId] = useState<number[]>([]); | ||
| const [isFooterActive, setIsFooterActiveFilter] = useState(false); |
There was a problem hiding this comment.
Try to keep the name of the variable and the setter with the same name.
DEMO LINK